Skip to content

Rename nWayUnion => multiwayMerge and NWayUnion => MultiwayMerge#5619

Merged
andralex merged 1 commit intodlang:masterfrom
RazvanN7:Issue_6718_partial_fix
Jul 17, 2017
Merged

Rename nWayUnion => multiwayMerge and NWayUnion => MultiwayMerge#5619
andralex merged 1 commit intodlang:masterfrom
RazvanN7:Issue_6718_partial_fix

Conversation

@RazvanN7
Copy link
Collaborator

@RazvanN7 RazvanN7 commented Jul 17, 2017

And adds: alias nWayUnion = multiwayMerge. Also the documentation is updated accordingly.

This is a partial fix to Issue 6718[1]

[1] https://issues.dlang.org/show_bug.cgi?id=6718

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is almost there.

assert(equal(multiwayMerge(a), witness));
}

alias nWayUnion = multiwayMerge;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, also please add alias NWayUnion = NWayMerge and mention it in the doc. Forgot to mention that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git push please

@RazvanN7 RazvanN7 force-pushed the Issue_6718_partial_fix branch from fda0f64 to e471b22 Compare July 17, 2017 12:05
}

alias nWayUnion = multiwayMerge;
alias NWayUnion = MultiwayMerge;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah there it is

@wilzbach
Copy link
Contributor

Q: Why do we need the nWay prefixes? Can't this by added to union as variadic overload?

union is currently not exposed because of a similar naming issue (and the regarding deprecation cycle), but it will be exposed soon https://github.com/dlang/phobos/blob/master/std/algorithm/setops.d#L1266
#4249

@andralex
Copy link
Member

@wilzbach algorithms for multiple set intersection are very different from those that intersect a small fixed number of sets, and have their own theory, practice, and important applications. I figure it's good to expose them under a different name rather than under the one-argument version of setIntersection.

@andralex andralex merged commit e8ebd82 into dlang:master Jul 17, 2017
@andralex
Copy link
Member

I'll force merge this, looks like the SHA matter is spurious. cc @CyberShadow

@CyberShadow
Copy link
Member

Yes, as far as I can tell, the SHA mismatch errors are due to a bug in GitHub:
#5617 (comment)

Force-merging will probably not break master, so it should be fine if there are no obvious DDoc errors.

@CyberShadow
Copy link
Member

Force-merging will probably not break master, so it should be fine if there are no obvious DDoc errors.

Actually, I take that back.

The SHA mismatch error indicates that there is a mismatch between the commits on the website and the commits in git. This potentially means that the tested change and the merged change are not the same! I don't know how the other CIs fetch the commits to test, but if potentially they use the same method as DAutoTest but do not verify that the fetched PR's head commit SHA1 coincides with the SHA1 from the API, then they might be testing the wrong changes.

So, I suggest being cautious about merging PRs with SHA1 mismatch errors for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants